Skip to content

Conversation

@ajantha-bhat
Copy link
Member

@ajantha-bhat ajantha-bhat commented Mar 28, 2025

Please feel free to checkout this PR and run locally.
Verified gradle clean build and all the tests are passing.

Changes:

TODO: (In the follow up PRs)

  • Once Polaris release available, add tests for polaris
  • Update readme to use polaris examples
  • Finalize release version
  • Add automated release workflow
  • Double check LICENSE and NOTICE (add a separate one for CLI runtime jar distribution)
  • Bump versions (like Iceberg, Nessie etc)
  • Migrate open issues/proposals from original repo
  • Build enhancements (like dependency between javadoc and jandex tasks)

This PR also contains the contributions from

Co-authored-by: Robert Stupp [email protected]
Co-authored-by: Christopher Lambert [email protected]
Co-authored-by: Harsh Maheshwari [email protected]

@ajantha-bhat
Copy link
Member Author

cc: @jbonofre, @snazy, @dimas-b

@mansehajsingh
Copy link
Contributor

mansehajsingh commented Mar 28, 2025

Should there be a gradle-wrapper.jar in gradle/wrapper/? Trying to run a gradle build fails unlike in the original iceberg-catalog-migrator project, where gradle/wrapper/gradle-wrapper.jar exists.

Failure:

Error: Could not find or load main class org.gradle.wrapper.GradleWrapperMain
Caused by: java.lang.ClassNotFoundException: org.gradle.wrapper.GradleWrapperMain

@ajantha-bhat
Copy link
Member Author

@mansehajsingh: Thanks for taking a look. I have addressed the comments.

Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the PR LGTM.
My request for changes is due to the addition of a .jar file and mention of the donation.

I also think, that the migrator should actually live in its own directory, as this repository is rather generic ("tools"). I propose to either move everything to iceberg-catalog-migrator/ or to have a dedicated Git repo for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gradle-wrapper.jar should be removed. Binary files, especially those having code, are not good practice.
There's a mechanism in the main repo to work around that.

uses: gradle/actions/setup-gradle@v4

- name: Build & Check
run: ./gradlew --rerun-tasks assemble ${{ env.ADDITIONAL_GRADLE_OPTS }} check publishToMavenLocal --scan
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
run: ./gradlew --rerun-tasks assemble ${{ env.ADDITIONAL_GRADLE_OPTS }} check publishToMavenLocal --scan
run: ./gradlew --rerun-tasks assemble ${{ env.ADDITIONAL_GRADLE_OPTS }} check publishToMavenLocal

No scan yet - see apache/polaris#475

Comment on lines 53 to 55
# since the `nessieQuarkusApp` gradle plugin expects the below variable
env:
JDK17_HOME: ${{ env.JAVA_HOME_17_X64 }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be superfluous

NOTICE Outdated
The Apache Software Foundation (http://www.apache.org/).

The initial code for the Polaris project was donated
to the ASF by Snowflake Inc. (https://www.snowflake.com/) copyright 2024.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be adopted

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. Updated.

@@ -0,0 +1,94 @@
@rem
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gradlew.bat file needs to be removed to work with the non-gradle-wrapper-jar mechanism

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this file

@ajantha-bhat
Copy link
Member Author

@snazy: thanks for the review. I have updated the gradle stuff to be similar to polaris main repo and verified ./gradlew clean build and handled other comments.

@ajantha-bhat
Copy link
Member Author

And regarding this

I also think, that the migrator should actually live in its own directory, as this repository is rather generic ("tools"). I propose to either move everything to iceberg-catalog-migrator/ or to have a dedicated Git repo for it.

The modules related to catalog migrator is under iceberg-catalog-migrator/ already. Do you mean move other folders like bom, buildsrc, codestyle also to iceberg-catalog-migrator/? I think these can be common for other tools that will be hosted in this repository.

Copy link
Member

@jbonofre jbonofre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First quick pass, overall ok, a couple of nits.

I'm also checking build and generated artifacts (in terms of legal).

@jbonofre
Copy link
Member

By the way, I would propose to have it in a dedicated repo (as we will have more tools coming in the repo, e.g. benchmark).

@jbonofre
Copy link
Member

Overall the PR LGTM. My request for changes is due to the addition of a .jar file and mention of the donation.

I also think, that the migrator should actually live in its own directory, as this repository is rather generic ("tools"). I propose to either move everything to iceberg-catalog-migrator/ or to have a dedicated Git repo for it.

We agreed on polaris-tools repo. If we change the plan, it has to be discussed again.
I propose to move forward as the approved plan and move later if needed.

@mansehajsingh
Copy link
Contributor

And regarding this

I also think, that the migrator should actually live in its own directory, as this repository is rather generic ("tools"). I propose to either move everything to iceberg-catalog-migrator/ or to have a dedicated Git repo for it.

The modules related to catalog migrator is under iceberg-catalog-migrator/ already. Do you mean move other folders like bom, buildsrc, codestyle also to iceberg-catalog-migrator/? I think these can be common for other tools that will be hosted in this repository.

+1 to this - would be super helpful if other additions to the repo were able to share these and deduplicate the work needed to bootstrap a new tool.

@ajantha-bhat
Copy link
Member Author

Hi everyone, Thanks for the reviews.

I have addressed all the comments.
As per TODO listed in PR description, we can have follow up PRs.

I would like to squash the PR once I have approval to add coauthored by to the commit message.
Please check.

@ajantha-bhat
Copy link
Member Author

Also merging this can help for #2 to avoid duplicate work of introducing build and infra related stuffs.

@ajantha-bhat ajantha-bhat mentioned this pull request Apr 2, 2025
Copy link
Contributor

@mansehajsingh mansehajsingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accidentally submitted approval on my other account.

@ajantha-bhat
Copy link
Member Author

I squashed the commit to add co-author by in the commit message.
( I have a backup. If people want to still review it individually again).

@ajantha-bhat
Copy link
Member Author

@snazy: Could you please take another look?

Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the polaris-tools repository is meant to host multiple and independent projects, I think that all those sub-projects should live in their own subfolder and be completely independent.

Therefore, everything, except the .github/ folder and the README.md + SECURITY.md files should go under the iceberg-catalog-migrator/ folder.

@@ -0,0 +1,94 @@
@rem
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this file

- Copied Main branch of nessie catalog migrator #bca310f
- Replace headers
- Rename packages
- Update security.md
- Remove automated release workflow related to projectnessie accounts
- Clean up Dremio keyword
- Adopt to polaris tools repo

This PR also contains the contributions from

Co-authored-by: Robert Stupp [email protected]
Co-authored-by: Christopher Lambert [email protected]
Co-authored-by: Harsh Maheshwari [email protected]
@ajantha-bhat
Copy link
Member Author

ajantha-bhat commented Apr 10, 2025

Given that the polaris-tools repository is meant to host multiple and independent projects, I think that all those sub-projects should live in their own subfolder and be completely independent.
Therefore, everything, except the .github/ folder and the README.md + SECURITY.md files should go under the iceberg-catalog-migrator/ folder.

@snazy: Thanks again for the review. I was under the impression that each tool can share the same gradle and build stuffs. But if we are going with this approach is also fine for me as it can help in having individual release versions. I have updated the PR now. Thanks.

@ajantha-bhat ajantha-bhat force-pushed the catalog-migrator branch 3 times, most recently from 80ed633 to e7e3123 Compare April 10, 2025 18:25
Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, the build's failing:

FAILURE: Build failed with an exception.

* Where:
Settings file '/home/snazy/devel/polaris/polaris-tools/iceberg-catalog-migrator/settings.gradle.kts' line: 20

* What went wrong:
/home/snazy/devel/polaris/polaris-tools/iceberg-catalog-migrator/version.txt (No such file or directory)

That and a last name change and we should be good!

Don't forget to open an issue about add support for releases.


val baseVersion = file("version.txt").readText().trim()

rootProject.name = "polaris-tools"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rootProject.name = "polaris-tools"
rootProject.name = "polaris-iceberg-catalog-migrator"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@snazy
Copy link
Member

snazy commented Apr 11, 2025

Issue #5 created

@ajantha-bhat
Copy link
Member Author

Hm, the build's failing:

I verified locally before pushing that builds are passing. But due to .gitignore file, version.txt was not committed to the repo. Forcefully committed it and verified the build by checking out to a fresh repo.

I think PR is ready.

Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@snazy snazy merged commit ac337ab into apache:main Apr 12, 2025
flyrain added a commit to flyrain/polaris-tools that referenced this pull request Dec 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants